Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stats tracking using CVal for starboard storage and file writes f… #707

Merged
merged 2 commits into from
Jul 26, 2023

Conversation

aee-google
Copy link
Contributor

@aee-google aee-google commented Jun 26, 2023

…rom cobalt.

b/277761317

Change-Id: Iabc8bf97772cfa42d9f0f0425eeef91376e4044d

@aee-google aee-google force-pushed the cval-sb-write branch 5 times, most recently from a62a143 to 2674c61 Compare June 26, 2023 16:43
@sherryzy
Copy link
Contributor

There are some related test failures: "Check failed: false. StatsTracker is not defined.". Could you fix that?

@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #707 (82c88a6) into main (810fb26) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##             main     #707   +/-   ##
=======================================
  Coverage   56.89%   56.89%           
=======================================
  Files        1898     1899    +1     
  Lines       94090    94110   +20     
=======================================
+ Hits        53528    53541   +13     
- Misses      40562    40569    +7     
Files Changed Coverage Δ
starboard/common/metrics/stats_tracker.h 78.57% <78.57%> (ø)
cobalt/storage/savegame_starboard.cc 59.22% <83.33%> (+1.49%) ⬆️

... and 7 files with indirect coverage changes

Copy link
Contributor

@sherryzy sherryzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good to me. Would love Kaido or Yavor to take a look as well.

starboard_api/api.cc Outdated Show resolved Hide resolved
starboard_api/api.cc Outdated Show resolved Hide resolved
Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how solid the idea of adding another API layer to call Starboard here looks like. It does look to me like most call sites could be trivially updated to simply use base::File that routes to Starboard underneath - except in very few limited places. @andrewsavage1 please have a look as well, wdyt ?

The stats tracking could then be entirely contained only in base::File with maybe another place or two.

starboard_api/stats_tracker.h Outdated Show resolved Hide resolved
cobalt/watchdog/watchdog_test.cc Outdated Show resolved Hide resolved
starboard_api/api.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@andrewsavage1 andrewsavage1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does look to me like most call sites could be trivially updated to simply use base::File that routes to Starboard underneath - except in very few limited places

I actually didn't see any places that couldn't use base::File, I definitely think that's a good option.

starboard_api/api.cc Outdated Show resolved Hide resolved
starboard_api/api.cc Outdated Show resolved Hide resolved
@aee-google
Copy link
Contributor Author

@kaidokert //base invokes file and storage record writes. CVal is defined in //cobalt/base.

starboard_api is possibly not the best name, but it's in the right place, I think.

  • //base can add stats data
  • CVal remains in //cobalt/base
  • CVal can be replaced or used in conjunction with other metrics collection implementations.

If I can limit starboard_api to just stats collection, I think that's probably better. It can still maybe depend on //starboard for convenience, but that's not strictly necessary.

@aee-google aee-google added cp-24.lts.1+ Cherry Pick to the 24.lts.1+ branch and removed cp-24.lts.1+ Cherry Pick to the 24.lts.1+ branch labels Jun 27, 2023
@aee-google aee-google force-pushed the cval-sb-write branch 2 times, most recently from 52c0dca to b8c6180 Compare July 6, 2023 12:43
@aee-google aee-google requested a review from kaidokert July 6, 2023 12:50
@kaidokert
Copy link
Member

So this is going in good direction, but now looking at it - would you be able to split out the PR that changes SbFile->base::File into a standalone submit ? That way we can quickly ensure that it landed as a no-op and didn't disrupt any of the more finicky platforms internally.

@aee-google
Copy link
Contributor Author

@kaidokert I split this up. This PR doesn't replace calls to SbFileWrite/SbFileWriteAll or usages of starboard::ScopedFile. I'll follow-up with multiple PRs to replace those with base::File.

Copy link
Member

@kaidokert kaidokert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise this looks okay to me now. Two things - now that @y4vor is back, i'd like him to have a look at the general concept. Note, the assumption is that we will only have base::File calling StarboardFile, all other call sites will be migrated to use base::File.

So really the two remaining call sites to increment those counters are going to be base/files/file_starboard.cc and cobalt/storage/savegame_starboard.cc.

Second, for some reason i'm not very keen on having a top-level starboard_stats/ module. I logically understand it should be above starboard/ and below base/ in the chain of dependencies. I'd still love to move it to starboard/common or a little more tucked away.

I almost wonder if statstracker interface starboard_stats::StatsTracker class couldn't simply be replaced with two simple callback functions or observers ?

@aee-google aee-google requested a review from y4vor July 13, 2023 16:26
@aee-google
Copy link
Contributor Author

I'm okay with moving starboard_stats around. It's fairly self-contained.

starboard_stats/export.h Outdated Show resolved Hide resolved
base/files/file_starboard.cc Show resolved Hide resolved
starboard_stats/BUILD.gn Outdated Show resolved Hide resolved
…rom cobalt.

b/277761317

Change-Id: Iabc8bf97772cfa42d9f0f0425eeef91376e4044d
@aee-google aee-google merged commit 94c46bc into youtube:main Jul 26, 2023
350 of 351 checks passed
@aee-google aee-google deleted the cval-sb-write branch July 26, 2023 21:37
@aee-google aee-google added the cp-24.lts.1+ Cherry Pick to the 24.lts.1+ branch label Aug 3, 2023
cobalt-github-releaser-bot pushed a commit that referenced this pull request Aug 3, 2023
#707)

…rom cobalt.

b/277761317

Change-Id: Iabc8bf97772cfa42d9f0f0425eeef91376e4044d
(cherry picked from commit 94c46bc)
aee-google added a commit that referenced this pull request Aug 3, 2023
#707)

…rom cobalt.

b/277761317

Change-Id: Iabc8bf97772cfa42d9f0f0425eeef91376e4044d
(cherry picked from commit 94c46bc)
aee-google added a commit that referenced this pull request Aug 4, 2023
#707)

…rom cobalt.

b/277761317

Change-Id: Iabc8bf97772cfa42d9f0f0425eeef91376e4044d
(cherry picked from commit 94c46bc)
aee-google added a commit that referenced this pull request Aug 4, 2023
…age and file writes f… (#1128)

Refer to the original PR: #707

…rom cobalt.

b/277761317

Change-Id: Iabc8bf97772cfa42d9f0f0425eeef91376e4044d

Co-authored-by: aee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cp-24.lts.1+ Cherry Pick to the 24.lts.1+ branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants